Fix: Finalize swept pending tx#999
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughScheduler config fields removed; scheduler selection now derives from Changes
Sequence Diagram(s)sequenceDiagram
participant OffchainTxHandler as "OffchainTx Handler\n(app)"
participant DB as "DB Service\n(load round/batch)"
participant Scheduler as "Scheduler\n(current time)"
participant Indexer as "Indexer/Notifier"
OffchainTxHandler->>DB: Load round by RootCommitmentTxId
DB-->>OffchainTxHandler: round or error (batch info)
OffchainTxHandler->>Scheduler: Query current time/state
Scheduler-->>OffchainTxHandler: now (for expiry checks)
OffchainTxHandler->>OffchainTxHandler: compute txSwept = (batch.SweepTxs exist) OR (load error treated swept) OR (ExpiryTimestamp < now)
OffchainTxHandler->>DB: create new vtxos with Swept = txSwept OR fallback(dust check)
DB-->>Indexer: emit TransactionEvent (with SpentVtxos, NewVtxos, Swept flags)
Indexer-->>Clients: notify (NotifyIncomingFunds etc.)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Review — #999Fix: Finalize swept pending tx SummaryWhen an offchain tx is finalized after its expiry, the new VTXOs may need to be marked as swept if the inputs were already swept by the server. Previously the code unconditionally set What ChangedSingle file:
Security Analysis✅ Safe fallback on error: ✅ Correct swept propagation: If the server swept the VTXO tree (inputs are swept), the outputs of a finalized tx built from those inputs should also be swept — they exist in the same expired tree. This is the right behavior.
Missing test coverageThe PR description mentions two needed test cases:
Neither is included in this PR. These would be valuable to add, especially the second case which verifies that non-swept expired txs produce spendable outputs. VerdictClean, minimal fix with correct safety semantics. The conservative error handling is the right call. Tests should follow to cover both swept and non-swept expiry scenarios. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service.go`:
- Around line 541-543: The code currently sets txSwept = err != nil after
calling s.vtxoStore.GetVtxos, which incorrectly treats DB read errors as
permanent swept state; remove the assignment and instead handle GetVtxos errors
explicitly (e.g., return the error up, log and skip processing, or retry) so
transient read failures don't mark vtxos swept; determine txSwept only from the
actual inputVtxos result (or explicit spent flags) after a successful call to
s.vtxoStore.GetVtxos rather than based on err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf6a5f5-cea2-48d9-b80f-15ff544c4aea
📒 Files selected for processing (1)
internal/infrastructure/db/service.go
|
@louisinger I added the 2 test cases you outlined in the PR description |
|
Iterative review — new commit Good test coverage for the swept-status propagation logic. Two cases are the right ones to test:
Observations:
Overall this is solid test coverage for a critical edge case. LGTM on the test additions. The core logic in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/infrastructure/db/service_test.go (1)
2201-2206: Preserve failure cause in async polling.If projection fails internally, this eventually check times out without surfacing the last DB error, which makes failures harder to triage.
💡 Proposed fix
+ var lastErr error require.Eventually(t, func() bool { vtxos, err := svc.Vtxos().GetVtxos(ctx, []domain.Outpoint{ {Txid: outTxid, VOut: 0}, }) - return err == nil && len(vtxos) > 0 - }, 5*time.Second, 100*time.Millisecond) + lastErr = err + return err == nil && len(vtxos) > 0 + }, 5*time.Second, 100*time.Millisecond, + "projected output vtxo not found (last error: %v)", lastErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service_test.go` around lines 2201 - 2206, The test's require.Eventually call hides the underlying error from svc.Vtxos().GetVtxos when the predicate keeps returning false; capture the last error inside the polling closure (e.g., a variable lastErr set to err each iteration) and after require.Eventually returns assert or fail with that captured error so the real DB/projection failure is surfaced; modify the test around the require.Eventually invocation that calls svc.Vtxos().GetVtxos to record the last err and then use require.NoError(lastErr) or t.Fatalf with lastErr if the eventually check still fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2036-2042: taprootPkScript currently ignores hex.DecodeString
errors and doesn't validate the decoded key length, which lets malformed fixture
hex produce truncated/zero-padded 34-byte scripts; change taprootPkScript to
validate input by handling the hex.DecodeString error and verifying len(pk) ==
32, and fail fast on invalid input (preferably by accepting a *testing.T
parameter and calling t.Fatalf with a clear message, or return an error) before
constructing the 34-byte script so malformed fixtures cause test failures rather
than silent false positives.
---
Nitpick comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2201-2206: The test's require.Eventually call hides the underlying
error from svc.Vtxos().GetVtxos when the predicate keeps returning false;
capture the last error inside the polling closure (e.g., a variable lastErr set
to err each iteration) and after require.Eventually returns assert or fail with
that captured error so the real DB/projection failure is surfaced; modify the
test around the require.Eventually invocation that calls svc.Vtxos().GetVtxos to
record the last err and then use require.NoError(lastErr) or t.Fatalf with
lastErr if the eventually check still fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d9b02fb-1af7-439d-8e20-aa647acb2151
📒 Files selected for processing (1)
internal/infrastructure/db/service_test.go
|
Iterative review — new commit Minor test hardening: LGTM for this increment. No concerns. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/infrastructure/db/service_test.go (1)
2072-2074: Prefer per-subtest decoder instance to avoid shared mutable state.Creating one decoder outside the loop makes subtests depend on mutable shared state. Instantiating it inside each
t.Runkeeps isolation tight and future-proofs fort.Parallel().♻️ Suggested refactor
- // The mock decoder is swapped per-subtest to return unique outpoints. - decoder := &mockTxDecoder{} - tests := []struct { name string inputSwept bool expectSwept bool @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + decoder := &mockTxDecoder{} // Fresh db per subtest for clean isolation. dbDir := t.TempDir() svc, err := db.NewService(db.ServiceConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service_test.go` around lines 2072 - 2074, The test creates a single shared decoder variable (decoder := &mockTxDecoder{}) outside the subtest loop which introduces shared mutable state; change the code to instantiate a fresh mockTxDecoder inside each t.Run (e.g., inside the subtest closure create decoder := &mockTxDecoder{}) so each subtest gets its own decoder instance, update any references in the subtest to use that local decoder, and ensure this enables safe use of t.Parallel() in the subtests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2122-2126: The mock decode function (decoder.decodeFn) currently
ignores its tx argument which can hide wiring bugs; update decoder.decodeFn to
assert the incoming tx equals the expected transaction payload used in the test
(e.g., compare to the test's raw/serialized tx variable) and fail the test if it
differs (using the test's t.Fatalf/t.Helper or require/assert helper). Keep the
returned outTxid, in1, in2 and outputs unchanged but add that assertion at the
start of decoder.decodeFn so wrong payloads cause a visible test failure.
---
Nitpick comments:
In `@internal/infrastructure/db/service_test.go`:
- Around line 2072-2074: The test creates a single shared decoder variable
(decoder := &mockTxDecoder{}) outside the subtest loop which introduces shared
mutable state; change the code to instantiate a fresh mockTxDecoder inside each
t.Run (e.g., inside the subtest closure create decoder := &mockTxDecoder{}) so
each subtest gets its own decoder instance, update any references in the subtest
to use that local decoder, and ensure this enables safe use of t.Parallel() in
the subtests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc5c8405-9aa8-45a8-a183-31c4b29ec4fa
📒 Files selected for processing (1)
internal/infrastructure/db/service_test.go
🔍 PR Review: Fix Finalize Swept Pending TXTL;DR: Solid bug fix. Prevents swept/expired input VTXOs from producing spendable output VTXOs during offchain tx finalization. What Changed
AnalysisThe bug: If a pending offchain tx was finalized after its inputs were swept (or expired past their timelock), the new output VTXOs were created without the The fix logic is sound:
One considerationThe Looking at the test: it generates 21 blocks (expiring the VTXO) without waiting for the server sweep. The input VTXO is
Could you clarify the expected path for "expired but not yet swept"? The code in this diff only sets TestsTwo well-structured e2e tests covering both swept scenarios. Good use of Verdict: The core fix is correct and addresses a real security concern. The "expired but not swept" edge case needs clarification on expected behavior. |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — #999
Fix: Finalize swept pending tx by @louisinger
PROTOCOL-CRITICAL — Requires human approval before merge
This PR touches VTXO state management (swept status propagation during offchain tx finalization). Bugs here could cause users to believe they have spendable funds that have already been swept, or conversely mark live VTXOs as swept. Human sign-off is mandatory.
Summary
When a pending offchain transaction is finalized after its inputs have expired (and potentially been swept by the server), the newly created output VTXOs must inherit the Swept flag. Previously, outputs were only marked swept based on dust-script detection. This PR adds logic to check whether the offchain tx is expired at finalization time, and if so, inspects input VTXOs to determine if they were swept.
Correctness
The core logic is sound:
- Decode inputs (previously discarded with
_) to look up input VTXOs. - If the tx has expired (
ExpiryTimestamp > 0 && time.Now().After(...)), fetch input VTXOs and check theirSweptflag. - If any input is swept, mark all outputs as swept.
- The
outputSweptvariable correctly preserves the existing dust-script check as a fallback whentxSweptis false.
The fix addresses a real gap: without it, a user finalizing a stale pending tx would receive VTXOs that appear spendable but whose underlying funds have already been claimed by the server's sweep.
Security Concerns
1. txSwept = err != nil — error-as-swept assumption (also flagged by CodeRabbit)
This is the most concerning line. If GetVtxos fails for a transient reason (DB timeout, connection blip), all outputs get permanently marked as swept. The comment says "it should never happen" but in production, transient DB errors do happen. This is a conservative-fail approach (better to falsely mark swept than falsely mark spendable), which is arguably the safer direction for fund safety. However, it means a transient DB error could render a user's funds inaccessible until manually corrected.
Recommendation: At minimum, log a warning when this path is taken so operators can detect it. Ideally, return the error and let the caller retry, rather than silently degrading.
2. Time-of-check vs. time-of-use on expiry
The code uses time.Now() to decide whether the tx is expired. In theory the sweep could happen between the time check and the GetVtxos call, but this is a negligible window and the consequence is only that outputs won't be marked swept — the next finalization attempt or a subsequent sweep check would catch it. Acceptable.
3. All-or-nothing sweep propagation
If any input is swept, all outputs are marked swept. This is correct for the Ark protocol because a partially-swept set of inputs means the entire transaction's on-chain anchoring is compromised. The checkpoint tx that commits these outputs depends on all inputs being valid.
Edge Cases
- Non-expired finalization: The
ExpiryTimestamp > 0 && time.Now().After(...)guard correctly skips the sweep check for non-expired txs. Normal flow is unaffected. GetVtxosreturns fewer VTXOs than inputs: If some input VTXOs are not found in the DB (the sqlite store skips outpoints with no rows), the returned slice will be shorter thanins. The code only checksinputVtxo.Swepton returned VTXOs, so missing VTXOs are treated as not-swept. This could be a gap — if an input VTXO is missing from the DB entirely, it might indicate it was already consumed/swept. Worth considering whether missing VTXOs should also trigger the swept flag.- Dust check override: When
txSweptis true, the dust check (IsSubDustScript) is skipped. This is correct — if the tx is swept, all outputs are swept regardless of dust status.
Test Coverage
The E2E tests cover both scenarios described in the PR:
- VTXOs already swept: Generates blocks, waits 30s for server sweep, verifies input is swept, finalizes, asserts output is swept. Good.
- VTXOs expired but not yet swept: Generates blocks but does NOT wait for server sweep, verifies input is NOT swept at query time, finalizes, asserts output is STILL marked swept.
The second test asserts require.True(t, incomingFunds[0].Swept) even when the input VTXO was verified as require.False(t, resp.Vtxos[0].Swept). This means the code marks outputs as swept even when inputs are not swept, purely based on some condition at finalization time. But looking at the production code, txSwept is only set to true if err != nil OR if inputVtxo.Swept is true. If inputs are not swept and no error occurred, txSwept remains false.
So how does the second test pass? The client-side check happens before finalization, but by the time the server processes it, the sweep may have already occurred (race between client's check and server's processing). This makes the "expired but not swept" test timing-dependent and potentially non-deterministic.
Missing from the PR description's ask: The description requested two test cases, one for "expired but not swept by the server." The existing test may rely on timing rather than deterministically testing the case where inputs are truly not swept at server finalization time.
Verdict
The fix is directionally correct and addresses a real safety gap. The conservative approach of marking outputs swept when in doubt is the right call for fund safety. However:
- The
err != nil => sweptfallback should at least log a warning so operators can detect transient DB failures being silently converted to swept status. - The behavior when
GetVtxosreturns fewer results than expected inputs deserves explicit handling (missing VTXOs could also indicate swept state). - The "expired but not swept" E2E test may not be deterministically testing what it claims.
This is protocol-critical code. Requesting human review from @altafan before merge.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/infrastructure/db/service.go (1)
536-544:⚠️ Potential issue | 🟠 MajorDB errors should not infer swept status.
The condition
txSwept = err != nil || ...at line 543 treats database read failures as if the transaction is swept. While the comment acknowledges this "should never happen," transient DB errors could incorrectly mark spendable vtxos as swept, permanently locking user funds.Consider handling the error explicitly rather than defaulting to swept:
Proposed fix
txSwept := false batch, err := s.roundStore.GetRoundWithCommitmentTxid(ctx, offchainTx.RootCommitmentTxId) - // We consider the tx swept if: - // - there is an error fetching the batch (this is just fallback, should never happen) - // - the batch is swept - // - the tx expired (meaning one or all its inputs expired and are already swept or about - // to be swept) - txSwept = err != nil || (batch != nil && len(batch.SweepTxs) > 0) || - time.Now().After(time.Unix(offchainTx.ExpiryTimestamp, 0)) + if err != nil { + log.WithError(err).Warnf( + "failed to fetch batch for offchain tx %s; cannot determine swept state", + offchainTx.ArkTxid, + ) + // Do not default to swept on DB errors - fall through with txSwept=false + } else if (batch != nil && len(batch.SweepTxs) > 0) || + time.Now().After(time.Unix(offchainTx.ExpiryTimestamp, 0)) { + txSwept = true + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service.go` around lines 536 - 544, The code currently sets txSwept to true when s.roundStore.GetRoundWithCommitmentTxid returns an error; instead, explicitly handle the error from GetRoundWithCommitmentTxid (s.roundStore.GetRoundWithCommitmentTxid) rather than inferring swept status: remove the err != nil clause from the txSwept boolean expression and add an explicit branch that handles err (e.g., return or propagate the error, or log and continue treating the TX as not swept), then compute txSwept only from (batch != nil && len(batch.SweepTxs) > 0) || time.Now().After(time.Unix(offchainTx.ExpiryTimestamp, 0)); keep references to txSwept, batch.SweepTxs, offchainTx.RootCommitmentTxId and offchainTx.ExpiryTimestamp so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/test/e2e/e2e_test.go`:
- Line 1143: Fix the typo in the comment in internal/test/e2e/e2e_test.go:
change the comment text "Givetime to the server to sweep the vtxo" to "Give time
to the server to sweep the vtxo" (update the comment near the relevant test
where the server sweep/wait is documented).
---
Duplicate comments:
In `@internal/infrastructure/db/service.go`:
- Around line 536-544: The code currently sets txSwept to true when
s.roundStore.GetRoundWithCommitmentTxid returns an error; instead, explicitly
handle the error from GetRoundWithCommitmentTxid
(s.roundStore.GetRoundWithCommitmentTxid) rather than inferring swept status:
remove the err != nil clause from the txSwept boolean expression and add an
explicit branch that handles err (e.g., return or propagate the error, or log
and continue treating the TX as not swept), then compute txSwept only from
(batch != nil && len(batch.SweepTxs) > 0) ||
time.Now().After(time.Unix(offchainTx.ExpiryTimestamp, 0)); keep references to
txSwept, batch.SweepTxs, offchainTx.RootCommitmentTxId and
offchainTx.ExpiryTimestamp so the change is localized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f18c877-9c06-4137-81ac-3320013fbd54
📒 Files selected for processing (9)
README.mddocker-compose.regtest.ymlenvs/arkd.dev.envenvs/arkd.light.envinternal/config/config.gointernal/core/application/service.gointernal/infrastructure/db/service.gointernal/test/e2e/e2e_test.gopkg/ark-lib/locktime.go
💤 Files with no reviewable changes (2)
- docker-compose.regtest.yml
- envs/arkd.dev.env
bitcoin-coder-bob
left a comment
bitcoin-coder-bob
left a comment
There was a problem hiding this comment.
left some questions about a race condition or overly broad swept marking
|
approved if CI passes |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/test/e2e/e2e_test.go (2)
2765-2784: These shorter sleeps are still blind waits around async state.Each of these sites immediately asserts on indexer or sweeper state after the sleep. Shortening the delay without switching to condition-based waits makes the suite load-sensitive and, in the dust-sweep case, can even pass before the automatic sweep attempt has actually been observed. Prefer polling the specific observable condition at each site instead of relying on fixed delays.
Also applies to: 3132-3145, 3219-3231, 3364-3407, 3467-3479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/e2e/e2e_test.go` around lines 2765 - 2784, Replace the blind time.Sleep calls with a short polling loop that repeatedly checks the actual observable condition until a timeout: after wg.Wait() poll alice.ListVtxos(ctx) and/or indexerSvc.GetVirtualTxs(ctx, []string{res.Txid}) instead of sleeping, and break when spendable contains an entry with Txid == res.Txid and indexerSvc returns a non-empty resp.Txs (set virtualTx from resp.Txs[0]); keep require.NoError checks and fail the test if the timeout elapses. Target the code using alice.ListVtxos, indexerSvc.GetVirtualTxs, res.Txid and virtualTx when implementing the polling so other similar blocks can be updated the same way.
1042-1135: Extract the pending-tx setup into a helper.Both new subtests repeat the same wallet/address/tapscript/build/sign/submit flow, and that same sequence already exists in the preceding
finalize pending txcase. A helper would keep these tests focused on the sweep/expiry behavior and reduce copy/paste drift.Also applies to: 1187-1280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/e2e/e2e_test.go` around lines 1042 - 1135, The tests duplicate the wallet/address/tapscript/build/sign/submit sequence; extract that repeated setup into a helper (e.g., preparePendingOffchainTx or setupPendingTxForSweep) that accepts (t *testing.T, ctx context.Context, explorer string) or similar and performs the steps currently done with setupArkSDKwithPublicKey, faucetOffchain, alice.FinalizePendingTxs, aliceWallet.GetAddresses, script.ParseVtxoScript, vtxoScript.ForfeitClosures, closure.Script, vtxoTapTree.GetTaprootMerkleProof, txscript.ParseControlBlock, build the tapscript, decode CheckpointTapscript, BuildTxs, checkpoint encoding, ptx.B64Encode and aliceWallet.SignTransaction then arkSvc.SubmitTx; have the helper return the minimal values callers need (e.g., txid, signedArkTx, encodedCheckpoints, alice/arkSvc cleanup functions or wallets if needed). Replace the duplicated blocks in the two subtests (and the earlier finalize pending tx case) with calls to that helper to keep tests focused on sweep/expiry behavior and avoid copy/paste drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/test/e2e/e2e_test.go`:
- Around line 1139-1155: The test uses a fixed time.Sleep after
generateBlocks(21) and then asserts transient vtxo states, which is race-prone;
replace the hard sleep and direct assertions with polling using
require.Eventually (or a similar retry loop) around alice.Indexer().GetVtxos to
wait for the expected final state (Swept==true) and, where you need to observe
the transient Spent==true && Swept==false window, add a test hook to the sweeper
(or a way to pause/observe it) and use require.Eventually to assert that
transient state while the sweeper is paused; specifically modify the blocks
around generateBlocks, time.Sleep, and the alice.Indexer().GetVtxos checks to
poll for the desired states instead of relying on fixed sleeps.
---
Nitpick comments:
In `@internal/test/e2e/e2e_test.go`:
- Around line 2765-2784: Replace the blind time.Sleep calls with a short polling
loop that repeatedly checks the actual observable condition until a timeout:
after wg.Wait() poll alice.ListVtxos(ctx) and/or indexerSvc.GetVirtualTxs(ctx,
[]string{res.Txid}) instead of sleeping, and break when spendable contains an
entry with Txid == res.Txid and indexerSvc returns a non-empty resp.Txs (set
virtualTx from resp.Txs[0]); keep require.NoError checks and fail the test if
the timeout elapses. Target the code using alice.ListVtxos,
indexerSvc.GetVirtualTxs, res.Txid and virtualTx when implementing the polling
so other similar blocks can be updated the same way.
- Around line 1042-1135: The tests duplicate the
wallet/address/tapscript/build/sign/submit sequence; extract that repeated setup
into a helper (e.g., preparePendingOffchainTx or setupPendingTxForSweep) that
accepts (t *testing.T, ctx context.Context, explorer string) or similar and
performs the steps currently done with setupArkSDKwithPublicKey, faucetOffchain,
alice.FinalizePendingTxs, aliceWallet.GetAddresses, script.ParseVtxoScript,
vtxoScript.ForfeitClosures, closure.Script, vtxoTapTree.GetTaprootMerkleProof,
txscript.ParseControlBlock, build the tapscript, decode CheckpointTapscript,
BuildTxs, checkpoint encoding, ptx.B64Encode and aliceWallet.SignTransaction
then arkSvc.SubmitTx; have the helper return the minimal values callers need
(e.g., txid, signedArkTx, encodedCheckpoints, alice/arkSvc cleanup functions or
wallets if needed). Replace the duplicated blocks in the two subtests (and the
earlier finalize pending tx case) with calls to that helper to keep tests
focused on sweep/expiry behavior and avoid copy/paste drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ab1b500-9bc0-472d-8674-a1ee07e6ace1
📒 Files selected for processing (2)
internal/core/application/service.gointernal/test/e2e/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/application/service.go
|
This PR is approved — ready to merge? |
| ).WithMetadata(errors.VtxoMetadata{VtxoOutpoint: vtxoOutpoint}) | ||
| } | ||
| if vtxo.Swept || !s.sweeper.scheduler.AfterNow(vtxo.ExpiresAt) { | ||
| if vtxo.Swept || now.After(time.Unix(vtxo.ExpiresAt, 0)) { |
There was a problem hiding this comment.
| if vtxo.Swept || now.After(time.Unix(vtxo.ExpiresAt, 0)) { | |
| if vtxo.Swept || vtxo.IsExpired() { |
let's move this logic to vtxo type so we can reuse it ? also cleaner to read
3 participants
we need 2 test cases :
@altafan please review
Summary by CodeRabbit
Documentation
Bug Fixes
Chores
Tests